Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

optionals to pointers + tests #1174

Merged
merged 3 commits into from
Sep 23, 2020
Merged

optionals to pointers + tests #1174

merged 3 commits into from
Sep 23, 2020

Conversation

vaikas
Copy link
Contributor

@vaikas vaikas commented Sep 18, 2020

Signed-off-by: Ville Aikas vaikas@vmware.com

Provide a description of what has been changed

Checklist

Fixes #1170

There are no user facing documents, since from the users point of view they are the same, just the clients changed. Also, add omitempty as it was missing from some of these. Add tests.

Since I'm not very familiar with the code style yet, I was curious about maybe changing the ResolveAuthRef to actually return an error in addition to map[string]string, string so it would be: (map[string]string, string, error) but didn't want to make that change without discussion first. I think that would be better for the callers to have an error to deal with rather than missing information?

zroubalik
zroubalik approved these changes Sep 21, 2020
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor nit in Changelog

CHANGELOG.md Outdated
@@ -35,6 +35,7 @@

### Improvements

- Change API optional structs to pointers to conform with k8s guide ([#1170](https://github.com/kedacore/keda/issues/1170))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please move this to ###Other section below, as this is not something that is directly affecting users.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing, done :)

@zroubalik
Copy link
Member

@ahmelsayed could you please take a look on the question? I think that it does make sense.

@vaikas
optionals to pointers + tests
bb81c6d 
Signed-off-by: Ville Aikas <vaikas@vmware.com>
@vaikas
oops, changelog
d92378f 
Signed-off-by: Ville Aikas <vaikas@vmware.com>
@vaikas
move the entry in Changelog to Other section
81366c4 
Signed-off-by: Ville Aikas <vaikas@vmware.com>
@ahmelsayed
Copy link
Contributor

@vaikas I think I didn't want ResolveAuthRef to cause a reconciliation error. if it could resolve the auth params, great, otherwise the scaler will fail later on if it indeed needed those params. In some services we try to reduce hard error paths and limit errors to the parts that need it. I'm okay with returning an error though as I think the user here might find it more useful to catch those configuration errors early rather than having to hunt through each scaler's error logs.

@ahmelsayed
Copy link
Contributor

If it's okay with you, we can merge this change and have that other change as a separate PR

@vaikas
Copy link
Contributor Author

vaikas commented Sep 22, 2020 •
edited
Loading

Yea, I'm fine with merging as is and doing more refactoring later on if we think it's warranted :) Just wanted to throw it out there. I can take a spin to see what it might look like and then bikeshed on that one :) Sound like a plan @ahmelsayed ?

@zroubalik zroubalik merged commit 72e924a into kedacore:v2 Sep 23, 2020
silenceper pushed a commit to silenceper/keda that referenced this pull request Oct 1, 2020
@vaikas @silenceper
optionals to pointers + tests (kedacore#1174)
44391a4 
* optionals to pointers + tests

Signed-off-by: Ville Aikas <vaikas@vmware.com>

* oops, changelog

Signed-off-by: Ville Aikas <vaikas@vmware.com>

* move the entry in Changelog to Other section

Signed-off-by: Ville Aikas <vaikas@vmware.com>
Signed-off-by: silenceper <silenceper@gmail.com>
SpiritZhou pushed a commit to SpiritZhou/keda that referenced this pull request Jul 18, 2023
@zroubalik
add --server-side to deploy because ScaledJob CRD is too long (kedaco…
c92d661 
…re#1174)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zroubalik zroubalik zroubalik approved these changes

@ahmelsayed ahmelsayed ahmelsayed approved these changes

Assignees

@ahmelsayed ahmelsayed

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

In the API types, optional structs should be pointers.
3 participants
@vaikas @zroubalik @ahmelsayed